Re: [PATCH v1 1/9] hw: encode accessing CPU index in MemTxAttrs

[Richard Henderson <richard.henderson@linaro.org>]

On 9/25/22 13:02, Alex Bennée wrote:
>> I'm not keen on adding another field like this.
> 
> Hmm I thought it was unavoidable from Edgar's comment:
> 
>    "CPU's can also have a Master-IDs (Requester IDs) which are unrelated to
>    the Clusters CPU index. This is used for example in the Xilinx ZynqMP
>    and Xilinx Versal and the XMPU (Memory Protection Units).
> 
>    Anyway, I think this approach is an improvement from the current state
>    but would rather see a new separate field from requester_id. Overloading
>    requester_id will break some of our use-cases (in the Xilinx tree)..."
> 
> Of course we don't have to care about external use cases but it seemed
> to indicate we might need both.

I missed that one.

>> What bounds our max number of cpus at the moment?  We use "int" in
>> struct CPUCore, but that's almost certainly for convenience.
>>
>> target/s390x/cpu.h:#define S390_MAX_CPUS 248
>> hw/i386/pc_piix.c:    m->max_cpus = HVM_MAX_VCPUS;
>>
>> hw/i386/pc_q35.c:    m->max_cpus = 288;
>>
>> hw/arm/virt.c:    mc->max_cpus = 512;
>>
>> hw/arm/sbsa-ref.c:    mc->max_cpus = 512;
>>
>> hw/i386/microvm.c:    mc->max_cpus = 288;
>>
>> hw/ppc/spapr.c:    mc->max_cpus = INT32_MAX;
>>
>>
>> Most of these are nicely bounded, but HVM_MAX_VCPUS is a magic number
>> from Xen, and ppc appears to be prepared for 31 bits worth of cpus.
> 
>  From 5642e4513e (spapr.c: do not use MachineClass::max_cpus to limit
> CPUs) I think it is being a little optimistic. Even with the beefiest
> hosts you start to see diminishing returns by ~12 vCPUs and it won't
> take long before each extra vCPU just slows you down.

Ok, I guess.  If we decided to add an assert that the cpuid fit in this field, we'd want 
to revisit this, I think.  Not an issue for the moment.

> I was confused by the last comment because I forgot the TLBs are not
> shared between cores. So I can just bang:
> 
>      MemTxAttrs attrs = { .cpu_index = cs->cpu_index };
> 
> into arm_cpu_tlb_fill and be done with it?

Yes, it looks like it.  I don't see any bulk overwrite of attrs in get_phys_addr and 
subroutines.  Mostly modifications of attrs->secure, as expected.


r~