Re: [PATCH 09/12] hw/vmapple/bdif: Introduce vmapple backdoor interface

[Alexander Graf <graf@amazon.com>]

On 16.06.23 12:39, Philippe Mathieu-Daudé wrote:
>
> On 15/6/23 00:56, Alexander Graf wrote:
>> The VMApple machine exposes AUX and ROOT block devices (as well as 
>> USB OTG
>> emulation) via virtio-pci as well as a special, simple backdoor platform
>> device.
>>
>> This patch implements this backdoor platform device to the best of my
>> understanding. I left out any USB OTG parts; they're only needed for
>> guest recovery and I don't understand the protocol yet.
>>
>> Signed-off-by: Alexander Graf <graf@amazon.com>
>> ---
>>   hw/vmapple/Kconfig        |   2 +
>>   hw/vmapple/bdif.c         | 245 ++++++++++++++++++++++++++++++++++++++
>>   hw/vmapple/meson.build    |   1 +
>>   hw/vmapple/trace-events   |   5 +
>>   include/hw/vmapple/bdif.h |  31 +++++
>
> Please enable scripts/git.orderfile if possible.


Sure, happy to :)


>
>> +#define REG_DEVID_MASK      0xffff0000
>> +#define DEVID_ROOT          0x00000000
>> +#define DEVID_AUX           0x00010000
>> +#define DEVID_USB           0x00100000
>> +
>> +#define REG_STATUS          0x0
>> +#define REG_STATUS_ACTIVE     BIT(0)
>> +#define REG_CFG             0x4
>> +#define REG_CFG_ACTIVE        BIT(1)
>> +#define REG_UNK1            0x8
>> +#define REG_BUSY            0x10
>> +#define REG_BUSY_READY        BIT(0)
>> +#define REG_UNK2            0x400
>> +#define REG_CMD             0x408
>> +#define REG_NEXT_DEVICE     0x420
>> +#define REG_UNK3            0x434
>
>
>> +static uint64_t bdif_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> +    uint64_t ret = -1;
>> +    uint64_t devid = (offset & REG_DEVID_MASK);
>> +
>> +    switch (offset & ~REG_DEVID_MASK) {
>> +    case REG_STATUS:
>> +        ret = REG_STATUS_ACTIVE;
>> +        break;
>> +    case REG_CFG:
>> +        ret = REG_CFG_ACTIVE;
>> +        break;
>> +    case REG_UNK1:
>> +        ret = 0x420;
>> +        break;
>> +    case REG_BUSY:
>> +        ret = REG_BUSY_READY;
>> +        break;
>> +    case REG_UNK2:
>> +        ret = 0x1;
>> +        break;
>> +    case REG_UNK3:
>> +        ret = 0x0;
>> +        break;
>> +    case REG_NEXT_DEVICE:
>> +        switch (devid) {
>> +        case DEVID_ROOT:
>> +            ret = 0x8000000;
>> +            break;
>> +        case DEVID_AUX:
>> +            ret = 0x10000;
>> +            break;
>> +        }
>> +        break;
>> +    }
>> +
>> +    trace_bdif_read(offset, size, ret);
>> +    return ret;
>> +}
>
>
>> +static const MemoryRegionOps bdif_ops = {
>> +    .read = bdif_read,
>> +    .write = bdif_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +    .valid = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 8,
>> +    },
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 8,
>
> IIUC your implementation is using (min, max) = (4, 4):
> i.e. if the guest emits a 64-bit read at offset 0, we want to return
> both REG_STATUS/REG_CFG registers.


I don't know if the BDIF device carries those semantics. Today, I'm only 
seeing 32bit accesses which is what I can vouch for. Will 8bit accesses 
go to a different register space or just access a subset of the 32bit 
register? I don't know :)

The same applies to 64bit ones. For all I know, they might as well end 
up as completely different registers.


Alex





Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879