Re: [Qemu-devel] [PATCH V2 3/7] introduce dpcd module.

[Peter Crosthwaite]

On Mon, Jul 6, 2015 at 9:30 AM, Frederic Konrad
<address@hidden> wrote:
> On 24/06/2015 08:44, Peter Crosthwaite wrote:
>>
>> On Mon, Jun 15, 2015 at 8:15 AM,  <address@hidden> wrote:
>>>
>>> From: KONRAD Frederic <address@hidden>
>>>
>>> This introduces a DPCD modules. It wires on a aux-bus and can be accessed
>>> by
>>
>> "module"
>>
>>> driver to get lane-speed, etc.
>>>
>>> Signed-off-by: KONRAD Frederic <address@hidden>
>>> ---
>>>   hw/display/Makefile.objs |   1 +
>>>   hw/display/dpcd.c        | 151
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   hw/display/dpcd.h        |  72 ++++++++++++++++++++++
>>>   3 files changed, 224 insertions(+)
>>>   create mode 100644 hw/display/dpcd.c
>>>   create mode 100644 hw/display/dpcd.h
>>>
>>> diff --git a/hw/display/Makefile.objs b/hw/display/Makefile.objs
>>> index 61c80f3..f75094f 100644
>>> --- a/hw/display/Makefile.objs
>>> +++ b/hw/display/Makefile.objs
>>> @@ -36,3 +36,4 @@ obj-$(CONFIG_VGA) += vga.o
>>>   common-obj-$(CONFIG_QXL) += qxl.o qxl-logger.o qxl-render.o
>>>
>>>   obj-$(CONFIG_VIRTIO) += virtio-gpu.o
>>> +obj-$(CONFIG_XLNX_ZYNQMP) += dpcd.o
>>
>> Make a DPCD config and add to aarch64 defconfig.
>>
>>> diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
>>> new file mode 100644
>>> index 0000000..b4eeea7
>>> --- /dev/null
>>> +++ b/hw/display/dpcd.c
>>> @@ -0,0 +1,151 @@
>>> +/*
>>> + * dpcd.c
>>> + *
>>> + *  Copyright (C)2015 : GreenSocs Ltd
>>
>> (C) 2015
>> (missing space)
>>
>>> + *      http://www.greensocs.com/ , email: address@hidden
>>> + *
>>> + *  Developed by :
>>> + *  Frederic Konrad   <address@hidden>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation, either version 2 of the License, or
>>> + * (at your option)any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> along
>>> + * with this program; if not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + */
>>> +
>>> +/*
>>> + * This is a simple AUX slave which emulates a connected screen.
>>> + */
>>> +
>>> +#include "hw/aux.h"
>>> +#include "dpcd.h"
>>> +
>>> +#ifndef DEBUG_DPCD
>>> +#define DEBUG_DPCD 0
>>> +#endif
>>> +
>>> +#define DPRINTF(fmt, ...) do {
>>> \
>>> +    if (DEBUG_DPCD) {
>>> \
>>> +        qemu_log("dpcd: " fmt, ## __VA_ARGS__);
>>> \
>>> +    }
>>> \
>>> +} while (0);
>>> +
>>> +#define DPCD_READABLE_AREA                      0x600
>>> +
>>> +struct DPCDState {
>>
>> /*< public >*/
>>
>>> +    AUXSlave parent_obj;
>>> +
>>
>> /*< private >*/
>>
>>> +    /*
>>> +     * The DCPD is 0x7FFFF length but read as 0 after offset 0x5FF.
>>> +     */
>>> +    uint8_t dpcd_info[DPCD_READABLE_AREA];
>>> +
>>> +    MemoryRegion iomem;
>>> +};
>>> +
>>> +static uint64_t dpcd_read(void *opaque, hwaddr offset, unsigned size)
>>> +{
>>> +    uint64_t ret;
>>
>> make a uint8_t
>>
>>> +    DPCDState *e = DPCD(opaque);
>>> +
>>> +    if (offset < DPCD_READABLE_AREA) {
>>> +        ret = e->dpcd_info[offset];
>>> +    } else {
>>> +        ret = 0;
>>> +    }
>>> +
>>> +    DPRINTF("read %u @0x%8.8lX\n", (uint8_t)ret, offset);
>>
>> to avoid this cast, and just let the implicit cast on the return
>> handle it for you.
>>
>> PRIx8
>> HWADDR_PRIx
>>
>>> +    return ret;
>>> +}
>>> +
>>> +static void dpcd_write(void *opaque, hwaddr offset, uint64_t value,
>>> +                       unsigned size)
>>> +{
>>> +    DPCDState *e = DPCD(opaque);
>>> +
>>> +    DPRINTF("write %u @0x%8.8lX\n", (uint8_t)value, offset);
>>> +
>>
>> PRIx8
>> HWADDR_PRIx
>>
>>> +    if (offset < DPCD_READABLE_AREA) {
>>> +        e->dpcd_info[offset] = value;
>>> +    }
>>
>> Should there be a else for a guest error?
>
>
> I think it's fine. Seems it's accessible but just read as 0.
>

So guest error is this semantic, it is not limited to only undefs, it
also applied to OOB registers that have a defined background. This
goal is to let the user know "the guest is doing something that
doesn't make sense" rather than trap undef behaviour.

Regards,
Peter

>>
>>> +}
>>> +
>>> +static const MemoryRegionOps aux_ops = {
>>> +    .read = dpcd_read,
>>> +    .write = dpcd_write,
>>> +    .valid = {
>>> +        .min_access_size = 1,
>>> +        .max_access_size = 1,
>>> +    },
>>> +    .impl = {
>>> +        .min_access_size = 1,