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 <th...@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
signature.asc
Description: OpenPGP digital signature
