Re: [PATCH v2 6/6] hw/isa/piix: Resolve redundant TYPE_PIIX3_XEN_DEVICE

[Chuck Zmudzinski <brchuckz@xxxxxxx>]

On 1/4/2023 5:35 PM, Philippe Mathieu-Daudé wrote:
> On 4/1/23 17:42, Chuck Zmudzinski wrote:
> > On 1/4/23 9:44 AM, Bernhard Beschow wrote:
> >> During the last patches, TYPE_PIIX3_XEN_DEVICE turned into a clone of
> >> TYPE_PIIX3_DEVICE. Remove this redundancy.
> >>
> >> Signed-off-by: Bernhard Beschow <shentey@xxxxxxxxx>
> >> ---
> >>   hw/i386/pc_piix.c             |  4 +---
> >>   hw/isa/piix.c                 | 20 --------------------
> >>   include/hw/southbridge/piix.h |  1 -
> >>   3 files changed, 1 insertion(+), 24 deletions(-)
> >>
> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> >> index 5738d9cdca..6b8de3d59d 100644
> >> --- a/hw/i386/pc_piix.c
> >> +++ b/hw/i386/pc_piix.c
> >> @@ -235,8 +235,6 @@ static void pc_init1(MachineState *machine,
> >>       if (pcmc->pci_enabled) {
> >>           DeviceState *dev;
> >>           PCIDevice *pci_dev;
> >> -        const char *type = xen_enabled() ? TYPE_PIIX3_XEN_DEVICE
> >> -                                         : TYPE_PIIX3_DEVICE;
> >>           int i;
> >>   
> >>           pci_bus = i440fx_init(pci_type,
> >> @@ -250,7 +248,7 @@ static void pc_init1(MachineState *machine,
> >>                                          : pci_slot_get_pirq);
> >>           pcms->bus = pci_bus;
> >>   
> >> -        pci_dev = pci_new_multifunction(-1, true, type);
> >> +        pci_dev = pci_new_multifunction(-1, true, TYPE_PIIX3_DEVICE);
> >>           object_property_set_bool(OBJECT(pci_dev), "has-usb",
> >>                                    machine_usb(machine), &error_abort);
> >>           object_property_set_bool(OBJECT(pci_dev), "has-acpi",
> >> diff --git a/hw/isa/piix.c b/hw/isa/piix.c
> >> index 98e9b12661..e4587352c9 100644
> >> --- a/hw/isa/piix.c
> >> +++ b/hw/isa/piix.c
> >> @@ -33,7 +33,6 @@
> >>   #include "hw/qdev-properties.h"
> >>   #include "hw/ide/piix.h"
> >>   #include "hw/isa/isa.h"
> >> -#include "hw/xen/xen.h"
> >>   #include "sysemu/runstate.h"
> >>   #include "migration/vmstate.h"
> >>   #include "hw/acpi/acpi_aml_interface.h"
> >> @@ -465,24 +464,6 @@ static const TypeInfo piix3_info = {
> >>       .class_init    = piix3_class_init,
> >>   };
> >>   
> >> -static void piix3_xen_class_init(ObjectClass *klass, void *data)
> >> -{
> >> -    DeviceClass *dc = DEVICE_CLASS(klass);
> >> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> >> -
> >> -    k->realize = piix3_realize;
> >> -    /* 82371SB PIIX3 PCI-to-ISA bridge (Step A1) */
> >> -    k->device_id = PCI_DEVICE_ID_INTEL_82371SB_0;
> >> -    dc->vmsd = &vmstate_piix3;
> >> -}
> >> -
> >> -static const TypeInfo piix3_xen_info = {
> >> -    .name          = TYPE_PIIX3_XEN_DEVICE,
> >> -    .parent        = TYPE_PIIX_PCI_DEVICE,
> >> -    .instance_init = piix3_init,
> >> -    .class_init    = piix3_xen_class_init,
> >> -};
> >> -
> >>   static void piix4_realize(PCIDevice *dev, Error **errp)
> >>   {
> >>       ERRP_GUARD();
> >> @@ -534,7 +515,6 @@ static void piix3_register_types(void)
> >>   {
> >>       type_register_static(&piix_pci_type_info);
> >>       type_register_static(&piix3_info);
> >> -    type_register_static(&piix3_xen_info);
> >>       type_register_static(&piix4_info);
> >>   }
> >>   
> >> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> >> index 65ad8569da..b1fc94a742 100644
> >> --- a/include/hw/southbridge/piix.h
> >> +++ b/include/hw/southbridge/piix.h
> >> @@ -77,7 +77,6 @@ struct PIIXState {
> >>   OBJECT_DECLARE_SIMPLE_TYPE(PIIXState, PIIX_PCI_DEVICE)
> >>   
> >>   #define TYPE_PIIX3_DEVICE "PIIX3"
> >> -#define TYPE_PIIX3_XEN_DEVICE "PIIX3-xen"
> >>   #define TYPE_PIIX4_PCI_DEVICE "piix4-isa"
> >>   
> >>   #endif
> > 
> > 
> > This fixes the regression with the emulated usb tablet device that I 
> > reported in v1 here:
> > 
> > https://lore.kernel.org/qemu-devel/aed4f2c1-83f7-163a-fb44-f284376668dc@xxxxxxx/
> > 
> > I tested this patch again with all the prerequisites and now with v2 there 
> > are no regressions.
> > 
> > Tested-by: Chuck Zmudzinski <brchuckz@xxxxxxx>
>
> (IIUC Chuck meant to send this tag to the cover letter)
>

Is it customary to tag the cover letter instead? I thought it appropriate
to tag the last commit in the series because it best represents the actual
commit on which tests were carried out. Also, the cover letter does not
actually represent a real commit that can be tested, except maybe the
last commit in the series. I did read the document on submitting patches,
but I don't remember if it specifies how to tag a series of patches with
the Tested-by tag.

Kind regards,

Chuck