[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH V7 03/11] vpci/header: implement guest BAR register handlers


  • To: Oleksandr <olekstysh@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 28 Jul 2022 09:01:22 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=g8w0QFlNxtccf0wz4v93VCyvBgfGFiQ40dpDtjNXi2E=; b=kVExEsGt93fBiF8mCUdZQa6Iv0F8yPrFiCF/Yy6/0KIWa+GmEeTGm15tZFCcQLAj2/7514/vyLQZ38NJxpmDR2za/J2rCd2WD6P6qfLiitWDPh0WB2l6aRd8LoSrCGqp7Hzx2pwOFmn56DrPRV92XC2jWiLjUHeoJwdJvMlfhGqVRwwINhdjJ6TGBucCwlvFQ/6syeT1zUJIfP8IOeHxVSfHbkGc05NCtlNS0/zdpMslYqE6pufwqBu8N2yNsIY/JhN89CGSfEFGjijgPCxnVbigf06rU5lUJ3I+p68+JWflvFapvTchWr7y9vMiiid49pAXinKchKRjBaxgodnkGA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=mn3P0HPPn0r/iXIA6a80UvZn0Rj/VZTxnGNIvenEeEj/DnPpOVbGSgc1D9CJ9sihHab7zaIxQx+B2eycjFhx4523C3gdZsXpPXOQlr0KJDWzeg45KTXmAwCu1ZZUlH573MQk16KlLqAw7N22ilX32sxkdu86FfV7j5u2HmEP04DbIaXuMa1nowIuMsCDwyg726+9e5olxbkD2qEFYG3DDDIKK2VrVdzEQGceB7Ik6PD1ZOIiU4SvFdR20uL2EB3NmBdken7w1UJdfIKOzN6FpK6fXn7aFKMI3RXk8J9X2ndspaJBsGf2rqw5WS6LsKpNaKCFIXbCg+P/YA7gu7p80A==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 28 Jul 2022 07:01:39 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 27.07.2022 18:17, Oleksandr wrote:
> On 27.07.22 13:15, Jan Beulich wrote:
>> On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
>>> @@ -527,6 +592,17 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>>           if ( (val & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO )
>>>           {
>>>               bars[i].type = VPCI_BAR_IO;
>>> +
>>> +#ifndef CONFIG_X86
>>> +            if ( !is_hwdom )
>>> +            {
>>> +                rc = vpci_add_register(pdev->vpci, empty_bar_read, NULL,
>>> +                                       reg, 4, &bars[i]);
>>> +                if ( rc )
>>> +                    goto fail;
>>> +            }
>>> +#endif
>> Since long term this can't be correct, it wants a TODO comment put next
>> to it.
> 
> 
> Looking into the previous versions of this patch (up to V3) I failed to 
> find any changes in current version which hadn't been discussed (and 
> agreed in some form).
> 
> Could you please clarify what exactly can't be correct the long term, 
> for me to put the proper TODO here. Do you perhaps mean that TODO needs 
> to explain why we have to diverge?

If a device has I/O port ranges, then that's typically for a reason.
Drivers (in the guest) may therefore want to use those ranges to
communicate with the device. Imagine in particular a device without
any MMIO BARs, and with only I/O port one(s).

>>> @@ -553,34 +635,47 @@ static int cf_check init_bars(struct pci_dev *pdev)
>>>           bars[i].size = size;
>>>           bars[i].prefetchable = val & PCI_BASE_ADDRESS_MEM_PREFETCH;
>>>   
>>> -        rc = vpci_add_register(pdev->vpci, vpci_hw_read32, bar_write, reg, 
>>> 4,
>>> -                               &bars[i]);
>>> +        rc = vpci_add_register(pdev->vpci,
>>> +                               is_hwdom ? vpci_hw_read32 : guest_bar_read,
>>> +                               is_hwdom ? bar_write : guest_bar_write,
>>> +                               reg, 4, &bars[i]);
>>>           if ( rc )
>>> -        {
>>> -            pci_conf_write16(pdev->sbdf, PCI_COMMAND, cmd);
>>> -            return rc;
>>> -        }
>>> +            goto fail;
>>>       }
>>>   
>>> -    /* Check expansion ROM. */
>>> -    rc = pci_size_mem_bar(pdev->sbdf, rom_reg, &addr, &size, PCI_BAR_ROM);
>>> -    if ( rc > 0 && size )
>>> +    /* Check expansion ROM: we do not handle ROM for guests. */
>>> +    if ( is_hwdom )
>> This again can't be right long-term. Personally I'd prefer if the code
>> was (largely) left as is, with adjustments (with suitable TODO comments)
>> made on a much smaller scope only.
> 
> 
> I can revive a comment that Oleksandr Andrushchenko provided for earlier 
> version by transforming into TODO:
> 
> 
> ROM BAR is only handled for the hardware domain and for guest domains
> there is a stub: at the moment PCI expansion ROM handling is supported
> for x86 only and it might not be used by other architectures without
> emulating x86. Other use-cases may include using that expansion ROM before
> Xen boots, hence no emulation is needed in Xen itself. Or when a guest
> wants to use the ROM code which seems to be rare.

ROMs can contain other than x86 code. While reportedly mostly dead, EFI
bytecode was an example of an abstraction layer supporting arbitrary
architectures. Therefore a comment along these lines would be okay, but
personally I'd prefer it to be less verbose - along the lines of the
one to be supplied for the I/O port restriction.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.