Re: [PATCH 5/9] vpci/header: Implement guest BAR register handlers

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 06.09.21 17:31, Jan Beulich wrote:
> On 03.09.2021 12:08, Oleksandr Andrushchenko wrote:
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -400,12 +400,72 @@ static void bar_write(const struct pci_dev *pdev, 
>> unsigned int reg,
>>   static void guest_bar_write(const struct pci_dev *pdev, unsigned int reg,
>>                               uint32_t val, void *data)
>>   {
>> +    struct vpci_bar *bar = data;
>> +    bool hi = false;
>> +
>> +    if ( bar->type == VPCI_BAR_MEM64_HI )
>> +    {
>> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
>> +        bar--;
>> +        hi = true;
>> +    }
>> +    else
>> +        val &= PCI_BASE_ADDRESS_MEM_MASK;
>> +    bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
>> +    bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
> What you store here is not the address that's going to be used,

bar->guest_addr is never used directly to be reported to a guest.

The same as bar->addr is never used to write to real BAR.

It is always used as an initial value which is then modified to reflect

lower bits, e.g. BAR type and if prefetchable, so I think this is perfectly

fine to have it this way.

>   as
> you don't mask off the low bits (to account for the BAR's size).
> When a BAR gets written with all ones, all writable bits get these
> ones stored. The address of the BAR, aiui, really changes to
> (typically) close below 4Gb (in the case of a 32-bit BAR), which
> is why memory / I/O decoding should be off while sizing BARs.
> Therefore you shouldn't look for the specific "all writable bits
> are ones" pattern (or worse, as you presently do, the "all bits
> outside of the type specifier are ones" one) on the read path.
> Instead mask the value appropriately here, and simply return back
> the stored value from the read path.
"PCI LOCAL BUS SPECIFICATION, REV. 3.0", "IMPLEMENTATION NOTE

Sizing a 32-bit Base Address Register Example" says, that

"Software saves the original value of the Base Address register, writes
0 FFFF FFFFh to the register, then reads it back."

The same applies for 64-bit BARs. So what's wrong if I try to catch such

a write when a guest tries to size the BAR? The only difference is that

I compare as

         if ( (val & PCI_BASE_ADDRESS_MEM_MASK_32) == 
PCI_BASE_ADDRESS_MEM_MASK_32 )
which is because val in the question has lower bits cleared.

With that respect I see no obvious reason why we can't construct our code

as it is.

>
>>   }
>>   
>>   static uint32_t guest_bar_read(const struct pci_dev *pdev, unsigned int 
>> reg,
>>                                  void *data)
>>   {
>> -    return 0xffffffff;
>> +    struct vpci_bar *bar = data;
>> +    uint32_t val;
>> +    bool hi = false;
>> +
>> +    switch ( bar->type )
>> +    {
>> +    case VPCI_BAR_MEM64_HI:
>> +        ASSERT(reg > PCI_BASE_ADDRESS_0);
>> +        bar--;
>> +        hi = true;
>> +        /* fallthrough */
>> +    case VPCI_BAR_MEM64_LO:
>> +    {
> Please don't add braces to case blocks when they're not needed.
Sure
>
>> +        if ( hi )
>> +            val = bar->guest_addr >> 32;
>> +        else
>> +            val = bar->guest_addr & 0xffffffff;
>> +        if ( (val & PCI_BASE_ADDRESS_MEM_MASK_32) ==  
>> PCI_BASE_ADDRESS_MEM_MASK_32 )
> This is wrong when falling through to here from VPCI_BAR_MEM64_HI:
> All 32 bits need to be looked at.
Good catch, will fix
>   Yet as per the comment further
> up I think it isn't right anyway to apply the mask here.
>
> Also: Stray double blanks.
>
>> +        {
>> +            /* Guests detects BAR's properties and sizes. */
>> +            if ( hi )
>> +                val = bar->size >> 32;
>> +            else
>> +                val = 0xffffffff & ~(bar->size - 1);
>> +        }
>> +        if ( !hi )
>> +        {
>> +            val |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>> +            val |= bar->prefetchable ? PCI_BASE_ADDRESS_MEM_PREFETCH : 0;
>> +        }
>> +        bar->guest_addr &= ~(0xffffffffull << (hi ? 32 : 0));
>> +        bar->guest_addr |= (uint64_t)val << (hi ? 32 : 0);
>> +        break;
>> +    }
>> +    case VPCI_BAR_MEM32:
> Please separate non-fall-through case blocks by a blank line.
Will do
>
>> @@ -522,6 +582,13 @@ static int add_bar_handlers(struct pci_dev *pdev, bool 
>> is_hwdom)
>>               if ( rc )
>>                   return rc;
>>           }
>> +        /*
>> +         * It is neither safe nor secure to initialize guest's view of the 
>> BARs
>> +         * with real values which are used by the hardware domain, so assign
>> +         * all zeros to guest's view of the BARs, so the guest can perform
>> +         * proper PCI device enumeration and assign BARs on its own.
>> +         */
>> +        bars[i].guest_addr = 0;
> I'm afraid I don't understand the comment: Without memory decoding
> enabled, the BARs are simple registers (with a few r/o bits).

My first implementation was that bar->guest_addr was initialized with

the value of bar->addr (physical BAR value), but talking on IRC with

Roger he suggested that this might be a security issue to let guest

a hint about physical values, so then I changed the assignment to be 0.

Thus the comment

>
>> --- a/xen/include/xen/pci_regs.h
>> +++ b/xen/include/xen/pci_regs.h
>> @@ -103,6 +103,7 @@
>>   #define  PCI_BASE_ADDRESS_MEM_TYPE_64      0x04    /* 64 bit address */
>>   #define  PCI_BASE_ADDRESS_MEM_PREFETCH     0x08    /* prefetchable? */
>>   #define  PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL)
>> +#define  PCI_BASE_ADDRESS_MEM_MASK_32       (~0x0fU)
> Please don't introduce an identical constant that's merely of
> different type. (uint32_t)PCI_BASE_ADDRESS_MEM_MASK at the use
> site (if actually still needed as per the comment above) would
> seem more clear to me.
Ok, I thought type casting is a bigger evil here
>
> Jan

Thank you,

Oleksandr