Re: [PATCH v2] vfio/pci: Introduce x-pci-class-code option

[Tomita Moeko <tomitamoeko@gmail.com>]

On 2025/5/29 18:41, Tomita Moeko wrote:
> On 2025/5/29 2:30, Alex Williamson wrote:
>> On Wed, 28 May 2025 23:55:48 +0800
>> Tomita Moeko <tomitamoeko@gmail.com> wrote:
>>
>>> Introduce x-pci-class-code option to allow users to override PCI class
>>> code of a device, similar to the existing x-pci-vendor-id option. Only
>>> the lower 24 bits of this option are used, though a uint32 is used here
>>> for determining whether the value is valid and set by user.
>>>
>>> Additionally, to prevent exposing VGA ranges on non-VGA devices, the
>>> x-vga=on option requires x-pci-class-code is either unset or set to
>>> VGA controller class.
>>>
>>> This is mainly intended for IGD devices that expose themselves either
>>> as VGA controller (primary display) or Display controller (non-primary
>>> display). The UEFI GOP driver depends on the device reporting a VGA
>>> controller class code (0x030000).
>>>
>>> Signed-off-by: Tomita Moeko <tomitamoeko@gmail.com>
>>> ---
>>> v2:
>>> * Add vdev class code check in vfio_populate_vga().
>>> * Fix type in trace-events.
>>> Link: https://lore.kernel.org/all/20250524153102.19747-1-tomitamoeko@gmail.com/
>>>
>>>  hw/vfio/pci.c        | 25 +++++++++++++++++++++++++
>>>  hw/vfio/pci.h        |  1 +
>>>  hw/vfio/trace-events |  1 +
>>>  3 files changed, 27 insertions(+)
>>>
>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index b1250d85bf..d57cb7356e 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -2726,6 +2726,14 @@ bool vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
>>>          return false;
>>>      }
>>>  
>>> +    /* vdev class should be either unmodified (PCI_ANY_ID), or VGA controller */
>>> +    if ((vdev->class_code != PCI_ANY_ID) &&
>>> +        (vdev->class_code != (PCI_CLASS_DISPLAY_VGA << 8)) &&
>>> +        (vdev->class_code != (PCI_CLASS_NOT_DEFINED_VGA << 8))) {
>>> +        error_setg(errp, "vdev is not a VGA device");
>>> +        return false;
>>> +    }
>>
>> I think we should follow the scheme used for vendor_id and device_id to
>> populate the struct field when not specified.  That let's us use it
>> more easily and consistently for things like this.
> 
> Hi, Alex
> 
> The class code override takes place in vfio_pci_config_setup(), where
> is after vfio_populate_vga() is called in vfio_populate_device(). So
> I have to check if it equals to default or VGA class code here, and
> not initializing the sturct field with device value. If we decide to
> initialize it for other purpose, I personally think we should also
> save the subvendor/subdevice value as well.

It have been several weeks, wondering if there is further comments.

Thanks,
Moeko

>> I think we can ignore PCI_CLASS_NOT_DEFINED_VGA.  The PCI Local Bus
>> Specification 2.1, dated June 1, 1995 (earliest I can find on the SIG)
>> includes the VGA class code and specifies base class 0x00 is reserved
>> for compatibility with devices built before the base class field was
>> defined, so at least before 1995.  Also, neither the kernel or QEMU
>> is_vga helpers account for this, so they'd not have a VGA region or be
>> properly detected elsewhere.  Thanks,
>>
>> Alex
> 
> Got it, will remove this in v3.
> 
> Thanks,
> Moeko
>  
>>> +
>>>      if (!(reg_info->flags & VFIO_REGION_INFO_FLAG_READ) ||
>>>          !(reg_info->flags & VFIO_REGION_INFO_FLAG_WRITE) ||
>>>          reg_info->size < 0xbffff + 1) {
>>> @@ -3092,6 +3100,21 @@ static bool vfio_pci_config_setup(VFIOPCIDevice *vdev, Error **errp)
>>>                                                vdev->sub_device_id);
>>>      }
>>>  
>>> +    /*
>>> +     * Class code is a 24-bit value at config space 0x09. Allow overriding it
>>> +     * with any 24-bit value.
>>> +     */
>>> +    if (vdev->class_code != PCI_ANY_ID) {
>>> +        if (vdev->class_code > 0xffffff) {
>>> +            error_setg(errp, "invalid PCI class code provided");
>>> +            return false;
>>> +        }
>>> +        /* Higher 24 bits of PCI_CLASS_REVISION are class code */
>>> +        vfio_add_emulated_long(vdev, PCI_CLASS_REVISION,
>>> +                               vdev->class_code << 8, ~0xff);
>>> +        trace_vfio_pci_emulated_class_code(vbasedev->name, vdev->class_code);
>>> +    }
>>> +
>>>      /* QEMU can change multi-function devices to single function, or reverse */
>>>      vdev->emulated_config_bits[PCI_HEADER_TYPE] =
>>>                                                PCI_HEADER_TYPE_MULTI_FUNCTION;
>>> @@ -3489,6 +3512,8 @@ static const Property vfio_pci_dev_properties[] = {
>>>                         sub_vendor_id, PCI_ANY_ID),
>>>      DEFINE_PROP_UINT32("x-pci-sub-device-id", VFIOPCIDevice,
>>>                         sub_device_id, PCI_ANY_ID),
>>> +    DEFINE_PROP_UINT32("x-pci-class-code", VFIOPCIDevice,
>>> +                       class_code, PCI_ANY_ID),
>>>      DEFINE_PROP_UINT32("x-igd-gms", VFIOPCIDevice, igd_gms, 0),
>>>      DEFINE_PROP_UNSIGNED_NODEFAULT("x-nv-gpudirect-clique", VFIOPCIDevice,
>>>                                     nv_gpudirect_clique,
>>> diff --git a/hw/vfio/pci.h b/hw/vfio/pci.h
>>> index 5ce0fb916f..587eb8cc9a 100644
>>> --- a/hw/vfio/pci.h
>>> +++ b/hw/vfio/pci.h
>>> @@ -156,6 +156,7 @@ struct VFIOPCIDevice {
>>>      uint32_t device_id;
>>>      uint32_t sub_vendor_id;
>>>      uint32_t sub_device_id;
>>> +    uint32_t class_code;
>>>      uint32_t features;
>>>  #define VFIO_FEATURE_ENABLE_VGA_BIT 0
>>>  #define VFIO_FEATURE_ENABLE_VGA (1 << VFIO_FEATURE_ENABLE_VGA_BIT)
>>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>>> index e90ec9bff8..e8d585b49a 100644
>>> --- a/hw/vfio/trace-events
>>> +++ b/hw/vfio/trace-events
>>> @@ -46,6 +46,7 @@ vfio_pci_emulated_vendor_id(const char *name, uint16_t val) "%s 0x%04x"
>>>  vfio_pci_emulated_device_id(const char *name, uint16_t val) "%s 0x%04x"
>>>  vfio_pci_emulated_sub_vendor_id(const char *name, uint16_t val) "%s 0x%04x"
>>>  vfio_pci_emulated_sub_device_id(const char *name, uint16_t val) "%s 0x%04x"
>>> +vfio_pci_emulated_class_code(const char *name, uint32_t val) "%s 0x%06x"
>>>  
>>>  # pci-quirks.c
>>>  vfio_quirk_rom_in_denylist(const char *name, uint16_t vid, uint16_t did) "%s %04x:%04x"
>>
>