| Message ID | 592b4a2a2b00e21470bec1a2ecf259a64eb285b2.1406703720.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jul 30, 2014 at 09:01:59AM +0200, Jan Kiszka wrote: > We used to be able to address both the QEMU and the KVM APIC via "apic". > This doesn't work anymore. So we need to use their parent class to turn > off the vapic on machines that should not expose them. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> OK so this is intended for 2.2? In that case, how about creating a macro with type name, and using that? This way things don't break if we rename something again. > --- > hw/i386/pc_piix.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > index 9694f88..73ba77d 100644 > --- a/hw/i386/pc_piix.c > +++ b/hw/i386/pc_piix.c > @@ -645,7 +645,7 @@ static QEMUMachine pc_machine_v1_1 = { > .property = "class",\ > .value = stringify(PCI_CLASS_MEMORY_RAM),\ > },{\ > - .driver = "apic",\ > + .driver = "apic-common",\ > .property = "vapic",\ > .value = "off",\ > },{\ > -- > 1.8.1.1.298.ge7eed54
Il 30/07/2014 10:57, Michael S. Tsirkin ha scritto: > In that case, how about creating a macro with type name, > and using that? This way things don't break if we rename > something again. Don't we have warnings for that now? Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > Il 30/07/2014 10:57, Michael S. Tsirkin ha scritto: >> On Wed, Jul 30, 2014 at 09:01:59AM +0200, Jan Kiszka wrote: >>> We used to be able to address both the QEMU and the KVM APIC via "apic". >>> This doesn't work anymore. So we need to use their parent class to turn >>> off the vapic on machines that should not expose them. >>> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> >> >> >> OK so this is intended for 2.2? If yes, we should cc: qemu-stable. >> In that case, how about creating a macro with type name, >> and using that? This way things don't break if we rename >> something again. > > Don't we have warnings for that now? Warnings don't help much in cases like this: "apic" still exists and has the property, it's just not the device we want. Macros aren't foolproof, either. >>> --- >>> hw/i386/pc_piix.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>> index 9694f88..73ba77d 100644 >>> --- a/hw/i386/pc_piix.c >>> +++ b/hw/i386/pc_piix.c >>> @@ -645,7 +645,7 @@ static QEMUMachine pc_machine_v1_1 = { >>> .property = "class",\ >>> .value = stringify(PCI_CLASS_MEMORY_RAM),\ >>> },{\ >>> - .driver = "apic",\ >>> + .driver = "apic-common",\ >>> .property = "vapic",\ >>> .value = "off",\ >>> },{\ >>> -- >>> 1.8.1.1.298.ge7eed54 You could use TYPE_APIC_COMMON here. Including "hw/i386/apic_internal.h" for it would be not so nice, though.
Quoting Markus Armbruster (2014-07-30 06:19:36) > Paolo Bonzini <pbonzini@redhat.com> writes: > > > Il 30/07/2014 10:57, Michael S. Tsirkin ha scritto: > >> On Wed, Jul 30, 2014 at 09:01:59AM +0200, Jan Kiszka wrote: > >>> We used to be able to address both the QEMU and the KVM APIC via "apic". > >>> This doesn't work anymore. So we need to use their parent class to turn > >>> off the vapic on machines that should not expose them. > >>> > >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >> > >> > >> > >> OK so this is intended for 2.2? > > If yes, we should cc: qemu-stable. Ping for stable 2.1.1, freeze is on Wednesday > > >> In that case, how about creating a macro with type name, > >> and using that? This way things don't break if we rename > >> something again. > > > > Don't we have warnings for that now? > > Warnings don't help much in cases like this: "apic" still exists and has > the property, it's just not the device we want. Macros aren't > foolproof, either. > > >>> --- > >>> hw/i386/pc_piix.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >>> index 9694f88..73ba77d 100644 > >>> --- a/hw/i386/pc_piix.c > >>> +++ b/hw/i386/pc_piix.c > >>> @@ -645,7 +645,7 @@ static QEMUMachine pc_machine_v1_1 = { > >>> .property = "class",\ > >>> .value = stringify(PCI_CLASS_MEMORY_RAM),\ > >>> },{\ > >>> - .driver = "apic",\ > >>> + .driver = "apic-common",\ > >>> .property = "vapic",\ > >>> .value = "off",\ > >>> },{\ > >>> -- > >>> 1.8.1.1.298.ge7eed54 > > You could use TYPE_APIC_COMMON here. Including > "hw/i386/apic_internal.h" for it would be not so nice, though.
On 2014-09-02 17:11, Michael Roth wrote: > Quoting Markus Armbruster (2014-07-30 06:19:36) >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> Il 30/07/2014 10:57, Michael S. Tsirkin ha scritto: >>>> On Wed, Jul 30, 2014 at 09:01:59AM +0200, Jan Kiszka wrote: >>>>> We used to be able to address both the QEMU and the KVM APIC via "apic". >>>>> This doesn't work anymore. So we need to use their parent class to turn >>>>> off the vapic on machines that should not expose them. >>>>> >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>> >>>> >>>> >>>> OK so this is intended for 2.2? >> >> If yes, we should cc: qemu-stable. > > Ping for stable 2.1.1, freeze is on Wednesday Lost track of this: was I supposed to provide anything different, or did this just fall under the table? Jan > >> >>>> In that case, how about creating a macro with type name, >>>> and using that? This way things don't break if we rename >>>> something again. >>> >>> Don't we have warnings for that now? >> >> Warnings don't help much in cases like this: "apic" still exists and has >> the property, it's just not the device we want. Macros aren't >> foolproof, either. >> >>>>> --- >>>>> hw/i386/pc_piix.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c >>>>> index 9694f88..73ba77d 100644 >>>>> --- a/hw/i386/pc_piix.c >>>>> +++ b/hw/i386/pc_piix.c >>>>> @@ -645,7 +645,7 @@ static QEMUMachine pc_machine_v1_1 = { >>>>> .property = "class",\ >>>>> .value = stringify(PCI_CLASS_MEMORY_RAM),\ >>>>> },{\ >>>>> - .driver = "apic",\ >>>>> + .driver = "apic-common",\ >>>>> .property = "vapic",\ >>>>> .value = "off",\ >>>>> },{\ >>>>> -- >>>>> 1.8.1.1.298.ge7eed54 >> >> You could use TYPE_APIC_COMMON here. Including >> "hw/i386/apic_internal.h" for it would be not so nice, though. >
On Thu, Oct 02, 2014 at 09:27:41AM +0200, Jan Kiszka wrote: > On 2014-09-02 17:11, Michael Roth wrote: > > Quoting Markus Armbruster (2014-07-30 06:19:36) > >> Paolo Bonzini <pbonzini@redhat.com> writes: > >> > >>> Il 30/07/2014 10:57, Michael S. Tsirkin ha scritto: > >>>> On Wed, Jul 30, 2014 at 09:01:59AM +0200, Jan Kiszka wrote: > >>>>> We used to be able to address both the QEMU and the KVM APIC via "apic". > >>>>> This doesn't work anymore. So we need to use their parent class to turn > >>>>> off the vapic on machines that should not expose them. > >>>>> > >>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >>>> > >>>> > >>>> > >>>> OK so this is intended for 2.2? > >> > >> If yes, we should cc: qemu-stable. > > > > Ping for stable 2.1.1, freeze is on Wednesday > > Lost track of this: was I supposed to provide anything different, or did > this just fall under the table? > > Jan Yes, I think Michael expected an ACK for stable. Oh well. Would you like me to apply as is, or to rework this to avoid duplication of string names? > > > >> > >>>> In that case, how about creating a macro with type name, > >>>> and using that? This way things don't break if we rename > >>>> something again. > >>> > >>> Don't we have warnings for that now? > >> > >> Warnings don't help much in cases like this: "apic" still exists and has > >> the property, it's just not the device we want. Macros aren't > >> foolproof, either. > >> > >>>>> --- > >>>>> hw/i386/pc_piix.c | 2 +- > >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>> > >>>>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >>>>> index 9694f88..73ba77d 100644 > >>>>> --- a/hw/i386/pc_piix.c > >>>>> +++ b/hw/i386/pc_piix.c > >>>>> @@ -645,7 +645,7 @@ static QEMUMachine pc_machine_v1_1 = { > >>>>> .property = "class",\ > >>>>> .value = stringify(PCI_CLASS_MEMORY_RAM),\ > >>>>> },{\ > >>>>> - .driver = "apic",\ > >>>>> + .driver = "apic-common",\ > >>>>> .property = "vapic",\ > >>>>> .value = "off",\ > >>>>> },{\ > >>>>> -- > >>>>> 1.8.1.1.298.ge7eed54 > >> > >> You could use TYPE_APIC_COMMON here. Including > >> "hw/i386/apic_internal.h" for it would be not so nice, though. > > > > -- > Siemens AG, Corporate Technology, CT RTC ITP SES-DE > Corporate Competence Center Embedded Linux
On 2014-10-02 10:03, Michael S. Tsirkin wrote: > On Thu, Oct 02, 2014 at 09:27:41AM +0200, Jan Kiszka wrote: >> On 2014-09-02 17:11, Michael Roth wrote: >>> Quoting Markus Armbruster (2014-07-30 06:19:36) >>>> Paolo Bonzini <pbonzini@redhat.com> writes: >>>> >>>>> Il 30/07/2014 10:57, Michael S. Tsirkin ha scritto: >>>>>> On Wed, Jul 30, 2014 at 09:01:59AM +0200, Jan Kiszka wrote: >>>>>>> We used to be able to address both the QEMU and the KVM APIC via "apic". >>>>>>> This doesn't work anymore. So we need to use their parent class to turn >>>>>>> off the vapic on machines that should not expose them. >>>>>>> >>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>>>>> >>>>>> >>>>>> >>>>>> OK so this is intended for 2.2? >>>> >>>> If yes, we should cc: qemu-stable. >>> >>> Ping for stable 2.1.1, freeze is on Wednesday >> >> Lost track of this: was I supposed to provide anything different, or did >> this just fall under the table? >> >> Jan > > Yes, I think Michael expected an ACK for stable. > Oh well. > Would you like me to apply as is, or to rework this to avoid duplication > of string names? I don't mind, but I wouldn't refuse if you want to take care of the issue. Jan
On Thu, Oct 02, 2014 at 10:05:38AM +0200, Jan Kiszka wrote: > On 2014-10-02 10:03, Michael S. Tsirkin wrote: > > On Thu, Oct 02, 2014 at 09:27:41AM +0200, Jan Kiszka wrote: > >> On 2014-09-02 17:11, Michael Roth wrote: > >>> Quoting Markus Armbruster (2014-07-30 06:19:36) > >>>> Paolo Bonzini <pbonzini@redhat.com> writes: > >>>> > >>>>> Il 30/07/2014 10:57, Michael S. Tsirkin ha scritto: > >>>>>> On Wed, Jul 30, 2014 at 09:01:59AM +0200, Jan Kiszka wrote: > >>>>>>> We used to be able to address both the QEMU and the KVM APIC via "apic". > >>>>>>> This doesn't work anymore. So we need to use their parent class to turn > >>>>>>> off the vapic on machines that should not expose them. > >>>>>>> > >>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > >>>>>> > >>>>>> > >>>>>> > >>>>>> OK so this is intended for 2.2? > >>>> > >>>> If yes, we should cc: qemu-stable. > >>> > >>> Ping for stable 2.1.1, freeze is on Wednesday > >> > >> Lost track of this: was I supposed to provide anything different, or did > >> this just fall under the table? > >> > >> Jan > > > > Yes, I think Michael expected an ACK for stable. > > Oh well. > > Would you like me to apply as is, or to rework this to avoid duplication > > of string names? > > I don't mind, but I wouldn't refuse if you want to take care of the issue. > > Jan I've applied as-is for now, thanks. I think the use of strings in compat machine types is too fragile, we should have a header with type names, and reuse it through macros, but apic is not unique here. > -- > Siemens AG, Corporate Technology, CT RTC ITP SES-DE > Corporate Competence Center Embedded Linux
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 9694f88..73ba77d 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -645,7 +645,7 @@ static QEMUMachine pc_machine_v1_1 = { .property = "class",\ .value = stringify(PCI_CLASS_MEMORY_RAM),\ },{\ - .driver = "apic",\ + .driver = "apic-common",\ .property = "vapic",\ .value = "off",\ },{\
We used to be able to address both the QEMU and the KVM APIC via "apic". This doesn't work anymore. So we need to use their parent class to turn off the vapic on machines that should not expose them. Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> --- hw/i386/pc_piix.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)