Re: [Qemu-devel] [PATCH v3] hw/ppc/spapr: Implement the h_page_init hypercall

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4605 bytes --]

On 18.02.2016 01:43, David Gibson wrote:
> On Wed, Feb 17, 2016 at 05:45:42PM +0100, Thomas Huth wrote:
>> This hypercall either initializes a page with zeros, or copies
>> another page.
>> According to LoPAPR, the i-cache of the page should also be
>> flushed if using H_ICACHE_INVALIDATE or H_ICACHE_SYNCHRONIZE,
>> and the d-cache should be synchronized to the RAM if the
>> H_ICACHE_SYNCHRONIZE flag is used. For this, two new functions
>> are introduced, kvmppc_dcbst_range() and kvmppc_icbi()_range, which
>> use the corresponding assembler instructions to flush the caches
>> if running with KVM on Power. If the code runs with TCG instead,
>> the code only uses tb_flush(), assuming that this will be
>> enough for synchronization.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Ugh, sorry to nitpick, but I've hit one more little issue here.
> 
>> ---
>>  v3:
>>  - Change H_HARDWARE return value into H_PARAMETER (which should
>>    be the right one according to the LoPAPR spec)
>>  - The dcbst and icbi helpers now contain the for-loop, too
>>
>>  PS: I'll have a look at the missing entries in the ibm,hypertas
>>      property later, once this got merged.
>>
>>  hw/ppc/spapr_hcall.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  target-ppc/kvm_ppc.h | 36 +++++++++++++++++++++++++++--
>>  2 files changed, 98 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 6e9b6be..6343caa 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -386,6 +386,69 @@ static target_ulong h_set_xdabr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>>      return H_SUCCESS;
>>  }
>>  
>> +static target_ulong h_page_init(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>> +                                target_ulong opcode, target_ulong *args)
>> +{
>> +    target_ulong flags = args[0];
>> +    hwaddr dst = args[1];
>> +    hwaddr src = args[2];
>> +    hwaddr len = TARGET_PAGE_SIZE;
>> +    uint8_t *pdst, *psrc;
>> +
>> +    if (flags & ~(H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE
>> +                  | H_COPY_PAGE | H_ZERO_PAGE)) {
>> +        qemu_log_mask(LOG_UNIMP, "h_page_init: Bad flags (" TARGET_FMT_lx "\n",
>> +                      flags);
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    if (!is_ram_address(spapr, dst) || (dst & ~TARGET_PAGE_MASK) != 0) {
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    /* Map-in source */
>> +    if (flags & H_COPY_PAGE) {
>> +        if (!is_ram_address(spapr, src) || (src & ~TARGET_PAGE_MASK) != 0) {
>> +            return H_PARAMETER;
>> +        }
>> +        psrc = cpu_physical_memory_map(src, &len, 0);
>> +        if (!psrc || len != TARGET_PAGE_SIZE) {
>> +            return H_PARAMETER;
>> +        }
>> +    }
>> +
>> +    /* Map-in destination */
>> +    pdst = cpu_physical_memory_map(dst, &len, 1);
>> +    if (!pdst || len != TARGET_PAGE_SIZE) {
>> +        if (flags & H_COPY_PAGE) {
>> +            cpu_physical_memory_unmap(psrc, len, 0, 0);
>> +        }
>> +        return H_PARAMETER;
>> +    }
>> +
>> +    if (flags & H_ZERO_PAGE) {
>> +        memset(pdst, 0, len);
>> +    }
>> +    if (flags & H_COPY_PAGE) {
>> +        memcpy(pdst, psrc, len);
>> +        cpu_physical_memory_unmap(psrc, len, 0, len);
> 
> So, at least on my compiler version (Fedora 23) I get one of those
> irritating "variable may be used uninitialized" warnings here for
> psrc.
>
> The compiler is wrong, of course, but you could both prevent its
> confusion and make the code a little straightforward if you remove the
> multiple tests on flags.  I think you should be able to do that if you
> restructure as:
> 
>     map in dest
>     if H_COPY_PAGE
>        map in src
>        memcpy
>        unmap src
>     else if H_ZERO_PAGE
>        memset
>     cache sync
>     unmap dest

I did not get that compiler warning here, but you're right,
restructuring the code also makes sense for readabilty, , so I'll change
my patch accordingly.

>> +    }
>> +
>> +    if (kvm_enabled() && (flags & H_ICACHE_SYNCHRONIZE) != 0) {
>> +        kvmppc_dcbst_range(cpu, pdst, len);
>> +    }
>> +    if (flags & (H_ICACHE_SYNCHRONIZE | H_ICACHE_INVALIDATE)) {
>> +        if (kvm_enabled()) {
>> +            kvmppc_icbi_range(cpu, pdst, len);
>> +        } else {
>> +            tb_flush(CPU(cpu));
>> +        }
>> +    }
>> +
>> +    cpu_physical_memory_unmap(pdst, len, 1, len);
>> +    return H_SUCCESS;
>> +}

 Thomas




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]