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

[Chuck Zmudzinski <brchuckz@xxxxxxx>]

On 1/7/23 6:05 AM, Bernhard Beschow wrote:
> Am 7. Januar 2023 01:08:46 UTC schrieb Chuck Zmudzinski <brchuckz@xxxxxxx>:
> >On 1/6/23 6:04 PM, Chuck Zmudzinski wrote:
> >> On 1/6/23 2:08 PM, Chuck Zmudzinski wrote:
> >>> On 1/6/23 7:25 AM, Philippe Mathieu-Daudé wrote:
> >>>> On 6/1/23 12:57, Bernhard Beschow wrote:
> >>>>> 
> >>>>> 
> >>>>> Am 4. Januar 2023 15:35:33 UTC schrieb "Philippe Mathieu-Daudé" 
> >>>>> <philmd@xxxxxxxxxx>:
> >>>>>> +Markus/Thomas
> >>>>>>
> >>>>>> On 4/1/23 15:44, 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(-)
> >>>> 
> >>>> 
> >>>>>>>    -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;
> >>>>>>
> >>>>>> IIUC, since this device is user-creatable, we can't simply remove it
> >>>>>> without going thru the deprecation process.
> >>>>> 
> >>>>> AFAICS this device is actually not user-creatable since 
> >>>>> dc->user_creatable is set to false once in the base class. I think it 
> >>>>> is safe to remove the Xen class unless there are ABI issues.
> >>>> Great news!
> >>> 
> >>> I don't know if this means the device is user-creatable:
> >>> 
> >>> chuckz@bullseye:~$ qemu-system-i386 -device piix3-ide-xen,help
> >>> piix3-ide-xen options:
> >>>   addr=<int32>           - Slot and optional function number, example: 
> >>> 06.0 or 06 (default: -1)
> >>>   failover_pair_id=<str>
> >>>   multifunction=<bool>   - on/off (default: false)
> >>>   rombar=<uint32>        -  (default: 1)
> >>>   romfile=<str>
> >>>   x-pcie-extcap-init=<bool> - on/off (default: true)
> >>>   x-pcie-lnksta-dllla=<bool> - on/off (default: true)
> >>> 
> >>> Today I am running qemu-5.2 on Debian 11, so this output is for
> >>> qemu 5.2, and that version of qemu has a piix3-ide-xen device.
> >>> Is that this same device that is being removed? If so, it seems to
> >>> me that at least as of qemu 5.2, the device was user-creatable.
> >>> 
> >>> Chuck
> >> 
> >> Good news! It looks the device was removed as user-creatable since version 
> >> 5.2:
> >> 
> >> chuckz@debian:~$ qemu-system-i386-7.50 -device help | grep piix
> >> name "piix3-usb-uhci", bus PCI
> >> name "piix4-usb-uhci", bus PCI
> >> name "piix3-ide", bus PCI
> >> name "piix4-ide", bus PCI
> >> chuckz@debian:~$ qemu-system-i386-7.50-bernhard-v2 -device help | grep piix
> >> name "piix3-usb-uhci", bus PCI
> >> name "piix4-usb-uhci", bus PCI
> >> name "piix3-ide", bus PCI
> >> name "piix4-ide", bus PCI
> >> chuckz@debian:~$
> >> 
> >> The piix3-ide-xen device is not present either with or without Bernhard's 
> >> patches
> >> for current qemu 7.50, the development version for qemu 8.0
> >> 
> >> Cheers,
> >> 
> >> Chuck
> >
> >
> >I traced where the pciix3-ide-xen device was removed:
> >
> >It was 7851b21a81 (hw/ide/piix: Remove redundant "piix3-ide-xen" device 
> >class)
> >
> >https://gitlab.com/qemu-project/qemu/-/commit/7851b21a8192750adecbcf6e8780a20de5891ad6
> >
> >about six months ago. That was between 7.0 and 7.1. So the device being 
> >removed
> >here is definitely not user-creatable, but it appears that this piix3-ide-xen
> >device that was removed between 7.0 and 7.1 was user-creatable. Does that one
> >need to go through the deprecation process? Or, since no one has complained
> >it is gone, maybe we don't need to worry about it?
>
> Good point! Looks like it fell through the cracks...
>
> There are voices who claim that this device and its non-Xen counterpart 
> should have never been user-creatable in the firtst place:
> https://patchwork.kernel.org/project/qemu-devel/patch/20190718091740.6834-1-philmd@xxxxxxxxxx/

Of course, only the xen variant was removed, so only users of the
xen variant were affected by the removal of the device. Any affected
users probably just substituted the non-xen variant for the xen variant
in their machines and didn't experience any problems.

Kind regards,

Chuck