Philippe Mathieu-Daudé <phi...@redhat.com> writes: > On 2/21/19 10:38 AM, Peter Maydell wrote: >> On Thu, 21 Feb 2019 at 09:22, Markus Armbruster <arm...@redhat.com> wrote: >>> Double-checking... you want me to keep goto reset_flash, like this: >>> >>> @@ -623,8 +617,8 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr >>> offset, >>> pfl->wcycle = 0; >>> pfl->status |= 0x80; >>> } else { >>> - DPRINTF("%s: unknown command for \"write block\"\n", >>> __func__); >>> - PFLASH_BUG("Write block confirm"); >>> + qemu_log_mask(LOG_GUEST_ERROR, >>> + "unknown command for \"write block\"\n"); >>> goto reset_flash; >>> } >>> break; >> >> Yes. (We seem to handle most kinds of guest errors in programming >> the flash by reset_flash.) > > Oh I missed the context of the patch here. > > So for the case of the Multi-WRITE command (0xe8):
Since I'm a clueless idiot on pflash, I need to process your argument real slow, so I can write a commit message that doesn't document my cluelessness forever. We have a little state machine, and its state is encoded in pfl->wcycle. pfl->cmd, pfl->counter. I'm going to show it as (value of pfl->wcycle, value of pfl->cmd, value of pfl->counter) for brevity. We start with (0, don't care, don't care). A guest write sends us a width, an address, and a value. pflash_mem_write_with_attrs() does permission checking, and pflash_write() the actual work. We enter it with @offset, @value and @width holding the message. cmd = value; trace_pflash_write(offset, value, width, pfl->wcycle); if (!pfl->wcycle) { /* Set the device in I/O access mode */ memory_region_rom_device_set_romd(&pfl->mem, false); } @cmd is @value truncated to 8 bits. > 1/ On first write cycle we have > > - address = flash_page_address (we store it in pfl->counter) > - data = flash_command (0xe8: enter Multi-WRITE) switch (pfl->wcycle) { case 0: /* read mode */ switch (cmd) { [...] case 0xe8: /* Write to buffer */ DPRINTF("%s: Write to buffer\n", __func__); pfl->status |= 0x80; /* Ready! */ break; [...] pfl->wcycle++; pfl->cmd = cmd; break; Transition from (0, don't care, don't care) to (1, 0xE8, don't care). I can't see "we store it in pfl->counter". Note that the address (passed in @offset) is entirely ignored. > 2/ Second cycle: > > - address = flash_page_address > We should check it matches flash_page_address > of cycle 1/, but we don't. > - data: N > > "N is the number of elements (bytes / words / double words), > minus one, to be written to the write buffer. Expected count > ranges are N = 00h to N = 7Fh (e.g., 1 to 128 bytes) in 8-bit > mode, N = 00h to N = 003Fh in 16-bit mode, and N = 00h to > N = 1Fh in 32-bit mode. Bus cycles 3 and higher are for writing > data into the write buffer. The confirm command (D0h) is > expected after exactly N + 1 write cycles; any other command at > that point in the sequence will prevent the transfer of the > buffer to the array (the write will be aborted)." > > Instead of starting to write the data in a buffer, we write it > directly to the block backend. case 1: switch (pfl->cmd) { [...] case 0xe8: /* Mask writeblock size based on device width, or bank width if * device width not specified. */ if (pfl->device_width) { value = extract32(value, 0, pfl->device_width * 8); } else { value = extract32(value, 0, pfl->bank_width * 8); } DPRINTF("%s: block write of %x bytes\n", __func__, value); pfl->counter = value; pfl->wcycle++; break; [...] } break; Transition from (1, 0xE8, don't care) to (2, 0xE8, N), where N is passed in @value. Again, the address (passed in @offset) is ignored. Nothing is written to the block backend, yet. > Instead of starting to write from cycle 3+, we write now in 2, > and keep cycle count == 2 (pfl->wcycle) until all data is > written, where we increment at 3. case 2: switch (pfl->cmd) { case 0xe8: /* Block write */ if (!pfl->ro) { pflash_data_write(pfl, offset, value, width, be); } else { pfl->status |= 0x10; /* Programming error */ } Write to memory, with pflash_data_write(), but don't flush to the backend, yet. This is (guest-visibly!) wrong. It's not quite "instead of starting to write the data in a buffer, we write it directly to the block backend." Note that we happily accept any address and width. I suspect we should only accept consecutive addresses and consistent width. pfl->status |= 0x80; if (!pfl->counter) { hwaddr mask = pfl->writeblock_size - 1; mask = ~mask; DPRINTF("%s: block write finished\n", __func__); pfl->wcycle++; if (!pfl->ro) { /* Flush the entire write buffer onto backing storage. */ pflash_update(pfl, offset & mask, pfl->writeblock_size); } else { pfl->status |= 0x10; /* Programming error */ } } If this is the multi-write's last write, flush the entire write block to the backend *boggle*. pfl->counter--; break; [...] } break; Transition from (2, 0xE8, N) to (2, 0xE8, N-1) if N>0 (3, 0xE8, don't care) if N==0 > 3/ Cycles 3+ > > - address = word index (relative to the page address) > - data = word value > > We check for the CONFIRM command, and switch the device back > to READ mode (setting Status), or reset the device (which is > modelled the same way). > > Very silly: If the guest cancelled and never sent the CONFIRM > command, the data is already written/flushed back. case 3: /* Confirm mode */ switch (pfl->cmd) { case 0xe8: /* Block write */ if (cmd == 0xd0) { pfl->wcycle = 0; pfl->status |= 0x80; On CONFIRM, transition from (3, 0xE8, don't care) back to the intial state (0, don't care, don't care). } else { DPRINTF("%s: unknown command for \"write block\"\n", __func__); PFLASH_BUG("Write block confirm"); goto reset_flash; } On anything else, pea brain implodes, and we exit(1). break; The fact that we let a hack job like this anywhere near "secure boot" is either frightening or amusing, depending on one's level of cynicism towards virtual secure boot. > So back to the previous code snippet, regardless the value, this > is neither a hw_error() nor a GUEST_ERROR. This code is simply not > correct. Eventually the proper (polite) error message should be: > > qemu_log_mask(LOG_UNIMP, "MULTI_WRITE: Abort not implemented," > " the data is already written" > " on storage!\n" > "Nevertheless resetting the flash" > " into READ mode.\n"); Yup, makes sense. Perhaps we should also LOG_UNIMP on the first write of a multi-write already, because the guest-visible wrongness starts right there. You tell me. Two things left to do for this series: FIXME comments, and a commit message. Before I tackle them, I'll give you a chance to correct misunderstandings in my reasoning.