On 12/06/2025 16:57, Roger Pau Monné wrote:
> On Wed, Jun 11, 2025 at 07:26:06PM +0200, Anthony PERARD wrote:
>> On Tue, Jun 10, 2025 at 06:29:30PM +0200, Roger Pau Monne wrote:
>>> diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
>>> index c388899306c2..ddbff6fffc16 100644
>>> --- a/docs/man/xl.cfg.5.pod.in
>>> +++ b/docs/man/xl.cfg.5.pod.in
>>> @@ -2351,6 +2351,14 @@ Windows
>>> L<https://xenproject.org/windows-pv-drivers/>.
>>> Setting B<xen_platform_pci=0> with the default device_model "qemu-xen"
>>> requires at least QEMU 1.6.
>>>
>>> +
>>> +=item B<xenpci_bar_uc=BOOLEAN>
>>> +
>>> +B<x86 only:> Select whether the memory BAR of the Xen PCI device should
>>> have
>>> +uncacheable (UC) cache attribute set in MTRR.
>>
>> For information, here are different name used for this pci device:
>>
>> - man xl.cfg:
>> xen_platform_pci=<bool>
>> Xen platform PCI device
>> - QEMU:
>> -device xen-platform
>> in comments: XEN platform pci device
>> with pci device-id PCI_DEVICE_ID_XEN_PLATFORM
>> - EDK2 / OVMF:
>> XenIoPci
>> described virtual Xen PCI device
>> But XenIo is a generic protocol in EDK2
>> Before XenIo, the pci device would be linked to XenBus, and
>> loaded with PCI_DEVICE_ID_XEN_PLATFORM
>> - Linux:
>> Seems to be called "xen-platform-pci"
>>
>> Overall, this PCI device is mostly referenced as the Xen Platform PCI
>> device. So "xenpci" or "Xen PCI device" is surprising to me, and I'm not
>> quite sure what it is.
>
> I can do xen_platform_pci_bar_uc, but it seems a bit long.
>
>>
>>> +
>>> +Default is B<true>.
>>> +
>>> =item B<viridian=[ "GROUP", "GROUP", ...]> or B<viridian=BOOLEAN>
>>>
>>> The groups of Microsoft Hyper-V (AKA viridian) compatible enlightenments
>>> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
>>> index cc67b18c0361..cfd39cc37cdc 100644
>>> --- a/tools/firmware/hvmloader/pci.c
>>> +++ b/tools/firmware/hvmloader/pci.c
>>> @@ -116,6 +116,8 @@ void pci_setup(void)
>>> * experience the memory relocation bug described below.
>>> */
>>> bool allow_memory_relocate = 1;
>>> + /* Select the MTRR cache attribute of the xenpci device BAR. */
>>> + bool xenpci_bar_uc = false;
>>
>> This default value for `xenpci_bar_uc` mean that hvmloader changes
>> behavior compared to previous version, right? Shouldn't we instead have
>> hvmloader keep the same behavior unless the toolstack want to use the
>> new behavior? (Like it's done for `allow_memory_relocate`,
>> "platform/mmio_hole_size")
>>
>> It would just mean that toolstack other than `xl` won't be surprised by
>> a change of behavior.
>
> My plan was that we didn't need changes to XAPI to implement this new
> mode, but given the comment I will change to keep the previous
> behavior in absence of a xenstore node.
>
>>
>>> BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO !=
>>> PCI_COMMAND_IO);
>>> @@ -130,6 +132,12 @@ void pci_setup(void)
>>> printf("Relocating guest memory for lowmem MMIO space %s\n",
>>> allow_memory_relocate?"enabled":"disabled");
>>>
>>> + s = xenstore_read(HVM_XS_XENPCI_BAR_UC, NULL);
>>> + if ( s )
>>> + xenpci_bar_uc = strtoll(s, NULL, 0);
>>> + printf("XenPCI device BAR MTRR cache attribute set to %s\n",
>>> + xenpci_bar_uc ? "UC" : "WB");
>>> +
>>> s = xenstore_read("platform/mmio_hole_size", NULL);
>>> if ( s )
>>> mmio_hole_size = strtoll(s, NULL, 0);
>>> @@ -271,6 +279,44 @@ void pci_setup(void)
>>> if ( bar_sz == 0 )
>>> continue;
>>>
>>> + if ( !xenpci_bar_uc &&
>>> + ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
>>> + PCI_BASE_ADDRESS_SPACE_MEMORY) &&
>>> + vendor_id == 0x5853 &&
>>> + (device_id == 0x0001 || device_id == 0x0002) )
>>
>> We don't have defines for 0x5853 in the tree (and those device_id)?
>> Maybe introduce at least one for the vendor_id:
>>
>> These two names are use by QEMU, OVMF, Linux, for example.
>>
>> #define PCI_VENDOR_ID_XEN 0x5853
>> #define PCI_DEVICE_ID_XEN_PLATFORM 0x0001
>>
>> There's even PCI_DEVICE_ID_XEN_PLATFORM_XS61 in Linux
>
> You mean in the public headers?
>
> For Device IDs we have ranges allocated to downstream vendors, I'm not
> sure we want to keep track of whatever IDs they use for their devices.
> OTOH, not tracking those IDs here means OSes are likely to miss
> additions of new Xen platform PCI devices?
>
The devices starting from ID c000 are supposed to be xen_pvdevice, which
are separate device types that work differently from Xen platform PCI
devices. So I think you only need to check for
PCI_DEVICE_ID_XEN_PLATFORM{,_XS61} unless another platform PCI ID gets
defined some day.
> I could introduce public/pci.h to contain those IDs, but I would like
> consensus on what should be there, otherwise this patch will get
> stuck.
>
>>
>>> diff --git a/tools/firmware/hvmloader/util.c
>>> b/tools/firmware/hvmloader/util.c
>>> index 79c0e6bd4ad2..31b4411db7b4 100644
>>> --- a/tools/firmware/hvmloader/util.c
>>> +++ b/tools/firmware/hvmloader/util.c
>>> @@ -867,7 +867,7 @@ void hvmloader_acpi_build_tables(struct acpi_config
>>> *config,
>>> config->table_flags |= ACPI_HAS_HPET;
>>>
>>> config->pci_start = pci_mem_start;
>>> - config->pci_len = pci_mem_end - pci_mem_start;
>>> + config->pci_len = RESERVED_MEMBASE - pci_mem_start;
>>> if ( pci_hi_mem_end > pci_hi_mem_start )
>>> {
>>> config->pci_hi_start = pci_hi_mem_start;
>>> diff --git a/tools/libs/light/libxl_create.c
>>> b/tools/libs/light/libxl_create.c
>>> index 8bc768b5156c..962fa820faec 100644
>>> --- a/tools/libs/light/libxl_create.c
>>> +++ b/tools/libs/light/libxl_create.c
>>> @@ -313,6 +313,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>>> libxl_defbool_setdefault(&b_info->u.hvm.usb,
>>> false);
>>> libxl_defbool_setdefault(&b_info->u.hvm.vkb_device, true);
>>> libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci, true);
>>> + libxl_defbool_setdefault(&b_info->u.hvm.xenpci_bar_uc, true);
>>> libxl_defbool_setdefault(&b_info->u.hvm.pirq,
>>> false);
>>>
>>> libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
>>> diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c
>>> index 4d67b0d28294..60ec0354d19a 100644
>>> --- a/tools/libs/light/libxl_dom.c
>>> +++ b/tools/libs/light/libxl_dom.c
>>> @@ -819,6 +819,15 @@ static int hvm_build_set_xs_values(libxl__gc *gc,
>>> goto err;
>>> }
>>>
>>> + if (info->type == LIBXL_DOMAIN_TYPE_HVM &&
>>> + libxl_defbool_val(info->u.hvm.xenpci_bar_uc)) {
>>
>> I think this condition is wrong. You should always write the value of
>> xenpci_bar_uc into xenstore, or only write it if a value have been
>> selected. But I guess we already lost the information here about whether
>> the value is the default or not, and it's probably not important, so I
>> think you should always write the value.
>
> Indeed, whether the value is the default or the user-selected one is
> lost by the time we get here.
>
> I would adjust according to the above comments, but I would suggest we
> leave out the addition of the Xen platform PCI device IDs to a
> separate patch, as I fear it will block the patch otherwise.
>
> Thanks, Roger.
>
Best regards,
Ngoc Tu Dinh | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
